Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added collision visibility modes #47204

Closed

Conversation

Janglee123
Copy link
Contributor

doc/classes/TileMap.xml Outdated Show resolved Hide resolved
doc/classes/TileMap.xml Outdated Show resolved Hide resolved
doc/classes/TileMap.xml Outdated Show resolved Hide resolved
scene/2d/tile_map.cpp Outdated Show resolved Hide resolved
@Janglee123
Copy link
Contributor Author

Janglee123 commented Mar 21, 2021

@pouleyKetchoupp there might be a problem with exported project. Before debug_shapes was explicitly used with is_debugging_collisions_hint which is false if not DEBUG_ENABLED.
But now collision_visibility == COLLISION_VISIBILITY_MODE_ALWAYS_SHOW can overpass it and might allowing to draw collision shape in exported project.

@pouleyKetchoupp
Copy link
Contributor

@Janglee123 Good catch! In this case it might be better to make sure it's disabled whatever the mode is when DEBUG_ENABLED is not defined.

@dalexeev
Copy link
Member

@pouleyKetchoupp @groud Could you please take a look at related PR #39072?

Always show collision shapes in editor and game.
</constant>
<constant name="COLLISION_VISIBILITY_MODE_ALWAYS_HIDE" value="1" enum="CollisionVisibilityMode">
Always Hide collision shapes in editor and game.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Always Hide collision shapes in editor and game.
Always hide collision shapes in editor and game.

Comment on lines +380 to +382
<constant name="COLLISION_VISIBILITY_MODE_ALWAYS_SHOW" value="0" enum="CollisionVisibilityMode">
Always show collision shapes in editor and game.
</constant>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused by these options. There doesn't seem to be a way to show collision shapes in editor, but not in game? That would seem like the most common use case to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property is mostly a switch to debug Tilemap collision in both editor and game, with a default option that allows to switch it off on all tilemaps at the same time, but it's true that a 4th option for editor only could be useful too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me the expected behavior would be that it behaves like other collision shapes by default:

  • Always visible in the editor
  • Visible in game if "Visible Collision Shapes" is toggled

And then since it can be annoying to have tilemaps always show collision shapes in the editor, or in game with Visible Collision Shapes, there needs to be a way to turn it off.

So IMO a boolean as done initially should have been enough, but maybe its logic was not correct?

  • show_collision = true: collision shapes visible in editor, visible in game if collision debugging is enabled, otherwise not visible
  • show_collision = false: collision shapes not visible in editor nor in game, regardless of debug settings

Is there any other use case worth supporting via an enum?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior is what #46623 was originally doing, but it didn't allow hiding all tilemaps at once in the editor when several of them are used as layers, which could cause some problems (see #46623 (comment)).

This PR implements the solution proposed by @groud in #46623 (comment), which is to have more options instead of a boolean, so by default it can be controlled using "Visible Collision Shapes" option for all tilemaps.

A gizmo could be a less confusing alternative to enable/disable collision shapes in the editor, but I don't know if it's possible to have one that would affect only the collision from tilemaps, it seems you can have only one gizmo per node type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "Visible Collision Shapes" option is used to configure what is visible at runtime, not in the editor. Collision shapes are always visible in the editor, regardless of whether that option is enabled or not.

So IMO it makes things more confusing if this option starts being used to show/hide tilemap collision shapes in the editor, while still leaving non-tilemap collision shapes always visible.

If "Visible Collision Shapes" is turned off, tilemap collision shapes should never be visible at runtime (but the current documentation suggests for the enum values otherwise).

#46623 (comment) is a misunderstanding of what "Visible Collision Shapes" is intended for. For this user's use case, they should just disable collision shapes visibility for all the tilemaps they don't want to see.

OR we simply switch the boolean to false by default, and let users enable it for the tilemaps where they do want to see visible collision shapes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree again. IMO its special use case(#46623 (comment))

Copy link
Contributor Author

@Janglee123 Janglee123 Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akien-mga Old pr has this problem, let me know if we are not merging this one, I need to update the old one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea for this would be to make the "Show debug collision shape" settings smarter. And change it to a 3-options too:

  • Hide,
  • Show in editor,
  • Show in editor and in game,

The question is what to do with the shapes. If the idea is to override this setting, we might simply have instead those 4 options per shape:

  • Use global settings,
  • Hide,
  • Show in editor,
  • Show in editor and in game.

I believe this is kind of the most expected way to set all of this, as those 3 options (plus one to "inherit" settings) are basically 3 levels of debug. From the "In don't want to debug physics" to the "I have issues with physics, show me shapes in game".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OR we simply switch the boolean to false by default, and let users enable it for the tilemaps where they do want to see visible collision shapes.

Maybe this is actually the easiest option. This way it's disabled by default in both editor and game (which solves #46623 (comment)), and then users can set things individually:
-show_collision=false: always disabled in both game and editor
-show_collision=true: always enabled in editor, controlled by "Visible Collision Shapes" in game

So the only thing that would not be possible for now is to switch collisions on and off for all tilemaps at once in the editor, but that could be handled later with an improved gizmo system.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #47382 to switch it to opt-in.

akien-mga added a commit to akien-mga/godot that referenced this pull request Mar 30, 2021
@akien-mga
Copy link
Member

Superseded by #47382.

@akien-mga akien-mga closed this Mar 30, 2021
@Janglee123 Janglee123 deleted the tilemap-collision-show branch March 31, 2021 04:32
lekoder pushed a commit to KoderaSoftwareUnlimited/godot that referenced this pull request Apr 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants